-
Notifications
You must be signed in to change notification settings - Fork 264
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Test cleanup issue 1287 #1308
Test cleanup issue 1287 #1308
Conversation
…sts/integration/* and axelrod/tests/property.py
…tests/unit/test_match.py
There seems to be a merge conflict, can you rebase this on to master? |
…sts/integration/* and axelrod/tests/property.py
…tests/unit/test_match.py
…1287' into test-cleanup-issue-1287
Sorry about the merge conflicts, this was the first time I was exposed to merge conflicts. I rebased to master and fixed merge conflicts. The tests also passed on my end. |
… tests/strategies/test_ann.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a really nice clean up.
It looks like there is a few lines missed.
It also looks like your editor changed the import order of unrelated imports. I don't think we've been consistent about import order. But some of these changes add a lot of new lines, and I think we probably don't want to do that.
|
||
import axelrod | ||
import axelrod as axl | ||
from axelrod import AntiCycler, Cycler, EvolvableCycler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete this line and update references throughout file.
import random | ||
import unittest | ||
|
||
import axelrod as axl | ||
from axelrod import EvolvablePlayer, seed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete this line, and update.
import warnings | ||
|
||
import axelrod | ||
import axelrod as axl | ||
from axelrod import Game |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete line and update
@@ -1,22 +1,27 @@ | |||
import unittest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be space between each of these imports?
import numpy as np | ||
|
||
import axelrod as axl | ||
from axelrod import DefaultGame, Player, LimitedHistory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete and update
axelrod/tests/unit/test_moran.py
Outdated
import matplotlib.pyplot as plt | ||
|
||
import axelrod as axl | ||
from axelrod import ApproximateMoranProcess, MoranProcess, Pdf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete and update
axelrod/tests/unit/test_resultset.py
Outdated
from numpy import mean, nanmedian, std | ||
|
||
import axelrod as axl | ||
import axelrod.interaction_utils as iu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the issue. It looks like it was decide that only axelrod would be imported like this, and that you'd use axl.interaction_utils elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's right.
@@ -1,19 +1,29 @@ | |||
"""Tests for the main tournament class.""" | |||
|
|||
import unittest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spacing
from collections import Counter | ||
|
||
import axelrod | ||
import axelrod as axl | ||
import axelrod.interaction_utils as iu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete this line. Ditto from comment on other file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto from comment on other file.
Can you tell me which comment that you're talking about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import axelrod.interaction_utils as iu
It looks like it was decided to just import axelrod, and to replace iu with axelrod.interaction_utils.
axelrod/tests/unit/test_pickling.py
Outdated
import random | ||
import unittest | ||
|
||
import axelrod as axl | ||
import axelrod.strategy_transformers as st |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete this line. It looks like that is what was decided on another file.
Hey @gaffney2010! . Thank you for your helpful comments. I reordered the import statements so that they'd be consistent. However, you're right about adding new lines, so I'll update them as per your reviews. Furthermore, is it ok if the import statements be as in PEP 8 import rules, as explained by this answer on SO. This may possibly make the import statements more consistent in the future. |
Yes. |
I have updated the code as per the recent review. Imports are grouped according to the PEP-8 convention, and new lines between the import statements are removed |
This looks good aside from the three places where you have: I think it was decided in the issue to only import axelrod. |
Yes, I now understand what you meant to say. I'll fix it asap :) |
Thanks @sudarshan-parvatikar all looks good to me 👍 |
Thanks all for being so patient and helpful. 😄 |
I have refactored the code according to Issue:1287. I also re-ordered import statements to make them consistent across all files and ran tests. The tests passed and I thinks this should close Issue:1287.